Skip to content

Allow adjutment of router replica count#2600

Merged
VietND96 merged 4 commits intoSeleniumHQ:trunkfrom
joshfng:router-replicas
Jan 21, 2025
Merged

Allow adjutment of router replica count#2600
VietND96 merged 4 commits intoSeleniumHQ:trunkfrom
joshfng:router-replicas

Conversation

@joshfng
Copy link
Contributor

@joshfng joshfng commented Jan 20, 2025 •
edited by qodo-code-review bot
Loading

User description

Description

This enables HA for the router deployment

Motivation and Context

Allows the charts to be deployed without the router services going offline, breaking any in-flight selenium runs with a http error

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

PR Type

Enhancement

Description

  • Added support for configurable router replica count.

  • Updated router-deployment.yaml to use dynamic replica values.

  • Introduced a new replicas field in values.yaml.

Changes walkthrough 📝

Relevant files
Enhancement
router-deployment.yaml
Use dynamic replica count in router deployment                     

charts/selenium-grid/templates/router-deployment.yaml

  • Changed hardcoded replica count to a dynamic value.
  • Utilized .Values.components.router.replicas for replica configuration.
    		• +1/-1     
    values.yaml
    Add replicas configuration in values.yaml                               

    charts/selenium-grid/values.yaml

  • Added a new replicas field under components.router.
  • Set default replica count to 1.
    		• +1/-0     
    Need help?
  • Type /help how to ... in the comments thread for any question about Qodo Merge usage.
  • Check out the documentation for more information.
    •

    @CLAassistant
    Copy link

    CLAassistant commented Jan 20, 2025 •
    edited
    Loading

    CLA assistant check
    All committers have signed the CLA.

    @qodo-code-review
    Copy link
    Contributor

    qodo-code-review bot commented Jan 20, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ No major issues detected

    @qodo-code-review
    Copy link
    Contributor

    qodo-code-review bot commented Jan 20, 2025 •
    edited
    Loading

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Validate deployment replica count input

    Add input validation to ensure router replicas value is a positive integer. Without
    validation, setting negative or zero replicas could break the deployment.

    charts/selenium-grid/templates/router-deployment.yaml [17]

    -replicas: {{ .Values.components.router.replicas }}
+replicas: {{ max 1 (.Values.components.router.replicas | int) }}
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion adds crucial input validation to prevent deployment failures by ensuring the replica count is always a positive integer, which is essential for maintaining service availability and preventing misconfiguration issues.

    		8

    @VietND96
    Copy link
    Member

    VietND96 commented Jan 20, 2025

    Hi, thank you for your contribution. Do you foresee any component else that needs to do the same?

    @VietND96
    Update config README
    16bd59d 
    Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
    @VietND96
    Merge branch 'trunk' into router-replicas
    526f98e
    @joshfng
    Copy link
    Contributor Author

    joshfng commented Jan 20, 2025

    Hi, thank you for your contribution. Do you foresee any component else that needs to do the same?

    I'm not familiar enough with each component to be able to say for sure, but it might not hurt to make each of them configurable.

    I can run through and change them all if you'd like

    @VietND96
    Copy link
    Member

    VietND96 commented Jan 20, 2025

    Yeah, go ahead with your changes. I think this is a chance to move all replicas configurable from values

    @VietND96
    Add replicas to values.yaml for all component
    7551055 
    Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
    @VietND96 VietND96 merged commit e10551d into SeleniumHQ:trunk Jan 21, 2025
    19 of 26 checks passed
    @VietND96
    Copy link
    Member

    VietND96 commented Jan 21, 2025

    @joshfng, thank you! I added for the rest and will release a new chart

    @joshfng joshfng deleted the router-replicas branch January 21, 2025 12:26
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    No reviews

    Assignees

    No one assigned

    Labels

    Review effort [1-5]: 1

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    3 participants

    @joshfng @CLAassistant @VietND96